Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Rename Pydantic v1 methods to their v2 counterparts #17123

Merged
merged 8 commits into from
Dec 18, 2024

Conversation

SyntaxColoring
Copy link
Contributor

@SyntaxColoring SyntaxColoring commented Dec 17, 2024

Overview

Some trivial renames, for the Pydantic v1->v2 upgrade. Closes EXEC-1063.

Test Plan and Hands on Testing

Just CI.

Changelog

  • Do a bunch of global find+replaces to rename Pydantic methods according to their migration guide.

  • In every project that runs on a robot, try to configure pytest to fail a test if it detects that it raised a Pydantic deprecation warning. This is intended to prevent us from using the old method names, which are less type-safe.

    This appears to only work for some warnings, but it works for the warnings that I care about and understand, so I didn't look into it further.

Review requests

None in particular.

Risk assessment

Low.

Merging

This is based atop PR #14871. It should merge into edge shortly after that PR.

@SyntaxColoring SyntaxColoring requested a review from a team December 17, 2024 00:34
@SyntaxColoring SyntaxColoring added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Dec 17, 2024
@SyntaxColoring SyntaxColoring marked this pull request as ready for review December 17, 2024 15:44
@SyntaxColoring SyntaxColoring requested a review from a team as a code owner December 17, 2024 15:44
@SyntaxColoring SyntaxColoring requested a review from a team December 18, 2024 17:44
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so annoying, thank you!

Copy link
Contributor

@vegano1 vegano1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol not sure why they made these changes

Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoying but straightforward. Thanks for also adding an error to pick up using the old methods, I'll definitely make that mistake at least once.

@SyntaxColoring
Copy link
Contributor Author

SyntaxColoring commented Dec 18, 2024

lol not sure why they made these changes

I think they were trying to namespace all of their stuff under model_* so they'd have room to expand without stepping on user code.

Amusingly, model_* was an unfortunate prefix for them to choose because users wanted to use it for their own machine learning stuff. So they later narrowed the namespace a bit in pydantic/pydantic#10315.

sfoster1 added a commit that referenced this pull request Dec 18, 2024
# Overview

This updates our robot-stack Python projects from Pydantic v1 to v2.

Closes PLAT-326 and GitHub issue #13983.

Affected projects that run on the robot:

 - api
 - robot-server
 - shared-data
 - hardware
 - server-utils
 - system-server
 - performance-metrics (re-lock only, depends on shared-data)

Affected projects that only run in CI and on our laptops:

 - g-code-testing
 - hardware-testing
 - abr-testing


# Test Plan

When the https://github.com/Opentrons/buildroot and
https://github.com/Opentrons/oe-core changes are ready:

* [x] OT-2
* [x] Make sure all servers still boot without errors (`systemctl
status`)
    * [ ] No unexpected mismatches reported by `pip check`
* I get "robot-server 8.2.0 has requirement pydantic==2.9.0, but you
have pydantic 2.9.2." I think we can fix this in a follow-up.
* [x] Try running a protocol or something and make sure the server
doesn't return an error or take an obscenely long time to respond.
* [x] Flex
* [x] Make sure all servers still boot on a Flex without errors
(`systemctl status`)
    * [x] No unexpected mismatches reported by `pip check`
* [x] Try running a protocol or something and make sure the server
doesn't return an error or take an obscenely long time to respond.


Beyond that, this PR touches a million little things in a million little
ways, so it's difficult to test. We should try to merge it early in the
release cycle to give us time to shake things out.

# Changelog

See https://docs.pydantic.dev/latest/migration/#migration-guide for
everything that has changed from Pydantic v1 to v2.

The basic methodology of this PR is:

* Update `setup.py`, `Pipfile`, and `Pipfile.lock` files to a new
Pydantic version, trying to follow Unfortunately, FastAPI is tightly
coupled to Pydantic, so we need to update it too. The FastAPI bump is
kept minimal.
* Run [bump-pydantic](https://github.com/pydantic/bump-pydantic) on all
projects. This automatically does a lot of the grunt work, but it does
need manual follow-up.
* Manually fix up lots of little things
* Do global find+replaces for some [trivial
renames](https://docs.pydantic.dev/latest/migration/#changes-to-pydanticbasemodel).
I moved some of this to [a separate PR,
#17123](#17123), because the
GitHub web UI was struggling with the big diff.


# Review requests

- Do the setup.py, Pipfile, and Pipfile.lock files look good? We're
trying to align on a definition of "good"
[here](https://opentrons.atlassian.net/wiki/spaces/RPDO/pages/4671602797/Python+dependency+management).
- Do all Pydantic migrations look correct? Reference:
https://docs.pydantic.dev/latest/migration/#migration-guide
- Do the rewritten validators look correct? Some of these needed manual
intervention.
- Do the `= None` additions look correct? `bump-pydantic` added these
automatically to match prior parse behavior—see
https://docs.pydantic.dev/latest/migration/#required-optional-and-nullable-fields.
As far as I can tell, these additions are always safe. But defaulting to
`None` may not be what we actually want, e.g. it may not match the
underlying JSON schema.
- This was a long-lived PR that changed hands several times, so there
are definitely vestigial things left over from earlier attempts. If
something looks unexplained or out of place to you, please speak up.

# Risk assessment

## Performance

This *will,* at least in the short term, make robot-server take much
longer to start up, and make the tests much slower. This is a known
Pydantic v2 problem (pydantic/pydantic#6768
etc.). Earlier testing on a Flex found it slowed from 46s to 1m54s
(2.5x). I don't think we'll be able to get it back down to Pydantic v1
times, but some proofs of concept suggest that we can mitigate it to
only ~1.6x slower. There are some ideas in EXEC-1060.

## Correctness

High-risk due to the breadth of changes: storage reads and writes, HTTP
requests and responses, communication between the
`opentrons.protocol_api` and `opentrons.protocol_engine`, ...

* [Pydantic's type coercion behavior has
changed](https://docs.pydantic.dev/latest/migration/#validator-behavior-changes),
trending in the direction of doing less type coercion. This is a good
direction, but it is basically impossible to audit affected one Python
protocol in the snapshot tests—see the inline review comments below.

---------

Co-authored-by: Max Marrone <[email protected]>
Co-authored-by: Seth Foster <[email protected]>
Base automatically changed from chore_update-pydantic-v2 to edge December 18, 2024 21:19
@SyntaxColoring SyntaxColoring force-pushed the pydantic_resolve_rename_warnings branch from e8b1637 to 7bd7fa6 Compare December 18, 2024 21:51
@SyntaxColoring SyntaxColoring removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Dec 18, 2024
@SyntaxColoring SyntaxColoring merged commit a56e919 into edge Dec 18, 2024
56 checks passed
@SyntaxColoring SyntaxColoring deleted the pydantic_resolve_rename_warnings branch December 18, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants